refactor: java cache and topics integration tests should use random cache names #379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactors the cache and topics integration tests to use a random cache name instead of reading from the environment. This follows the JavaScript SDK pattern. We have re-written the tests to use a base test class that sets up the client. Test classes are responsible for setting up and tearing down the cache (if any). Because a test class now re-uses the same cache, we have refactored the tests to use random keys in each test case. These two changes have fixed race conditions that have caused consistent test failures when running locally. Lastly the auth tests were not closing clients in the tests. Because of this, warning messages appeared in the logs consistently. We have fixed these problems by closing clients between reuse.
Previously we created the test cache in each of the test classes. This was unnecessary and brings about more opportunity for developer error when writing new test classes. We lift that setup and teardown to the base class.
nand4011
reviewed
Jul 9, 2024
| @@ -160,14 +147,17 @@ void testBatchGetWithStringKeys_RequestsMoreThanMaxConn() { | |||
| .build(); | |||
|
|
|||
| // test data with more keys than max concurrent requests | |||
| Vector<String> keys = new Vector<>(); | |||
There was a problem hiding this comment.
Suggested change
| Vector<String> keys = new Vector<>(); | |
| final List<String> keys = new ArrayList<>(); |
Is there a specific reason to use a Vector? I'm not sure I've ever seen one of those in use.
There was a problem hiding this comment.
That's what I used in my intro to Java class 22 years ago. I can use ArrayList lol.
nand4011
reviewed
Jul 9, 2024
| if (System.getenv("TEST_CACHE_NAME") == null) { | ||
| throw new IllegalArgumentException( | ||
| "Integration tests require TEST_CACHE_NAME env var; see README for more details."); | ||
| credentialProvider = CredentialProvider.fromEnvVar("TEST_AUTH_TOKEN"); |
There was a problem hiding this comment.
Is it worth changing TEST_AUTH_TOKEN to MOMENTO_API_KEY in this pr? I thought we were slowly changing to that.
There was a problem hiding this comment.
I'm going to change the CI / env vars in a separate PR. This one is pretty busy already.
nand4011
reviewed
Jul 9, 2024
nand4011
approved these changes
Jul 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactors the cache and topics integration tests to use a random cache
name instead of reading from the environment. This follows the
JavaScript SDK pattern.
We have re-written the tests to use a base test class that sets up the
client, assigns a random cache name, creates the cache, and tears it
down at the end of tests.
Because a test class now re-uses the same cache, we have refactored
the tests to use random keys in each test case. These two changes have
fixed race conditions that have caused consistent test failures when
running locally.
Lastly the auth tests were not closing clients in the tests. Because
of this, warning messages appeared in the logs consistently. We have
fixed these problems by closing clients between reuse.